-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🌱 Refactor docker-push* Makefile targets so users can control with ALL_DOCKER_BUILD which images are pushed #8586
Conversation
Hi @AndiDog. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test cc @kubernetes-sigs/cluster-api-release-team |
Based on the about-to-be-upstreamed kubernetes-sigs#8586
Based on the about-to-be-upstreamed kubernetes-sigs#8586
Based on the about-to-be-upstreamed kubernetes-sigs#8586
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this change makes sense to me, however @AndiDog can you please rebase the PR?
Rebased by applying the patch also to the in-memory infrastructure provider Makefile targets |
/lgtm |
LGTM label has been added. Git tree hash: 6917729a11deff0e6df18e65c3da6d18eb2be75e
|
/unassign |
/lgtm |
/cc @sbueringer @killianmuldoon @CecileRobertMichon What do you all think about this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx looks good to me, just two questions
Otherwise ready for merge
Sorry for the long delay
Makefile
Outdated
@@ -694,6 +694,7 @@ docker-build-all: $(addprefix docker-build-,$(ALL_ARCH)) ## Build docker images | |||
docker-build-%: | |||
$(MAKE) ARCH=$* docker-build | |||
|
|||
# Choice of images to build/push | |||
ALL_DOCKER_BUILD = core kubeadm-bootstrap kubeadm-control-plane docker-infrastructure in-memory-infrastructure test-extension clusterctl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ALL_DOCKER_BUILD = core kubeadm-bootstrap kubeadm-control-plane docker-infrastructure in-memory-infrastructure test-extension clusterctl | |
ALL_DOCKER_BUILD ?= core kubeadm-bootstrap kubeadm-control-plane docker-infrastructure in-memory-infrastructure test-extension clusterctl |
Q: Do we have to add this to make it overwritable via env var?
I assume not, just no idea why we usually use ?=
for stuff like this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you do. Using ?=
allows the env var to take precedence if it's set whereas =
just ignores the env var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this would mean the PR wouldn't work in its current state, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made this configurable since fork builds (as my company's) may not require building all components. For example, only the controllers may be required but not the test or CLI images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this part. My question is, is it currently possible to overwrite this via an env var without changing the above to ?=
?
(basically a newbie Makefile question :))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
➜ export ALL_DOCKER_BUILD=core
➜ make docker-build
...
➜ /Library/Developer/CommandLineTools/usr/bin/make ARCH=arm64 docker-build-core docker-build-kubeadm-bootstrap docker-build-kubeadm-control-plane docker-build-docker-infrastructure docker-build-in-memory-infrastructure docker-build-test-extension docker-build-clusterctl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah sorry, I thought this was part of my changes already but it's a GitHub suggested change 🙈.
We definitely use this change with overridden ALL_DOCKER_BUILD
already, and make ALL_DOCKER_BUILD="core" ARCH=boom REGISTRY=myreg docker-push
on macOS works fine to build only the core image. I found it's because make
will override variables if you set them on the command line. Therefore, ALL_DOCKER_BUILD="core" ARCH=boom REGISTRY=myreg make docker-push
, i.e. using real environment variables only, doesn't work. I will quickly test again and switch multiple places to ?=
. Good find, thx!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thank you very much, that is good to know :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless you don't want to make environment vs. command line parameters the same. Not sure if that is desired?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good if both works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @Jont828
/lgtm |
@CecileRobertMichon I think this should work as is for any of our use cases since it would use the default values unless specified. |
…DOCKER_BUILD which images are pushed
Thank you very much!! /lgtm |
LGTM label has been added. Git tree hash: c2fb9a47e81fcf984db3b07617ec6e657f9af980
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/area devtools |
I don't expect any problems, but let's verify that everything works as expected by checking the logs of https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/post-cluster-api-push-images/1691699431717474304 |
Looks all good to me, e.g. ➜ docker buildx imagetools inspect gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217
Name: gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217
MediaType: application/vnd.docker.distribution.manifest.list.v2+json
Digest: sha256:01c215327091a4aeb0d0c34a60b4dbcdc4fc53e1aa03e5507000ae9dfc4d1b6d
Manifests:
Name: gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:420e6bd0557990e026abebda0f18aeec0a65c878fe7c48b19e726b49caf907ce
MediaType: application/vnd.docker.distribution.manifest.v2+json
Platform: linux/amd64
Name: gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:f6836cff88a704d87eb3c0164c4b8e5d0fda54a02764ed7eff3071d9303dccce
MediaType: application/vnd.docker.distribution.manifest.v2+json
Platform: linux/arm
Name: gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:34c7ae653098373a65d53b2756ea6249288484342c43a64916d54a25fe906e0b
MediaType: application/vnd.docker.distribution.manifest.v2+json
Platform: linux/arm64
Name: gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:9afbc9f3a88d037099f6e72ec9205d0abac42d8c96409684ad7f1a0d787c1795
MediaType: application/vnd.docker.distribution.manifest.v2+json
Platform: linux/ppc64le
Name: gcr.io/k8s-staging-cluster-api/cluster-api-controller:v20230816-v1.5.0-rc.0-174-gbb2688217@sha256:a09ace82c8d92ee5768820d436c1e84175f55bd3a7b8e031fb5863c8ccc93a41
MediaType: application/vnd.docker.distribution.manifest.v2+json
Platform: linux/s390x
|
What this PR does / why we need it:
Previously,
make docker-push-all
would try to push the combined manifest and image of each component, whilemake docker-build-all
would respectALL_DOCKER_BUILD
and only build the specified components. This change makes the Makefile target naming consistent (docker-push-<component>
anddocker-push-manifest-<component>
, clusterctl not specially named anymore) and refactors so thatALL_DOCKER_BUILD
defines which components are built and pushed.Specifically, this allows a build of only production components using
ALL_DOCKER_BUILD="core kubeadm-bootstrap kubeadm-control-plane"
.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):n/a